-
Notifications
You must be signed in to change notification settings - Fork 15.2k
libunwind: Remove OS requirements from tests to make them run on more OSes #167642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-libunwind Author: Gleb Popov (arrowd) ChangesThere might be a cleaner way to enable these tests running on FreeBSD, I'm open to suggestions. Full diff: https://github.com/llvm/llvm-project/pull/167642.diff 6 Files Affected:
diff --git a/libunwind/test/bad_unwind_info.pass.cpp b/libunwind/test/bad_unwind_info.pass.cpp
index 272a83f64a611..2fadcfefdecc3 100644
--- a/libunwind/test/bad_unwind_info.pass.cpp
+++ b/libunwind/test/bad_unwind_info.pass.cpp
@@ -10,7 +10,7 @@
// Ensure that libunwind doesn't crash on invalid info; the Linux aarch64
// sigreturn frame check would previously attempt to access invalid memory in
// this scenario.
-// REQUIRES: target={{(aarch64|s390x|x86_64)-.+linux.*}}
+// REQUIRES: target={{(aarch64|s390x|x86_64)-.+(linux|freebsd).*}}
// GCC doesn't support __attribute__((naked)) on AArch64.
// UNSUPPORTED: gcc
diff --git a/libunwind/test/eh_frame_fde_pc_range.pass.cpp b/libunwind/test/eh_frame_fde_pc_range.pass.cpp
index 795ce66806f28..e86f80839a655 100644
--- a/libunwind/test/eh_frame_fde_pc_range.pass.cpp
+++ b/libunwind/test/eh_frame_fde_pc_range.pass.cpp
@@ -13,7 +13,7 @@
// clang-format off
-// REQUIRES: target={{x86_64-.+-linux-gnu}}
+// REQUIRES: target={{x86_64-.+-(linux-gnu|freebsd.*)}}
// REQUIRES: objcopy-available
// TODO: Figure out why this fails with Memory Sanitizer.
@@ -21,7 +21,7 @@
// RUN: %{build}
// RUN: %{objcopy} --dump-section .eh_frame_hdr=%t_ehf_hdr.bin %t.exe
-// RUN: echo -ne '\xFF' | dd of=%t_ehf_hdr.bin bs=1 seek=2 count=2 conv=notrunc status=none
+// RUN: printf '\377' | dd of=%t_ehf_hdr.bin bs=1 seek=2 count=2 conv=notrunc status=none
// RUN: %{objcopy} --update-section .eh_frame_hdr=%t_ehf_hdr.bin %t.exe
// RUN: %{exec} %t.exe
diff --git a/libunwind/test/forceunwind.pass.cpp b/libunwind/test/forceunwind.pass.cpp
index 9e032fc680806..f3ad08c2cf693 100644
--- a/libunwind/test/forceunwind.pass.cpp
+++ b/libunwind/test/forceunwind.pass.cpp
@@ -7,7 +7,7 @@
//
//===----------------------------------------------------------------------===//
-// REQUIRES: linux
+// REQUIRES: linux || freebsd
// TODO: Figure out why this fails with Memory Sanitizer.
// XFAIL: msan
diff --git a/libunwind/test/remember_state_leak.pass.sh.s b/libunwind/test/remember_state_leak.pass.sh.s
index d3335cf82290b..0e0723f66d3c8 100644
--- a/libunwind/test/remember_state_leak.pass.sh.s
+++ b/libunwind/test/remember_state_leak.pass.sh.s
@@ -6,7 +6,7 @@
#
#===------------------------------------------------------------------------===#
-# REQUIRES: target={{x86_64-.+-linux-gnu}}
+# REQUIRES: target={{x86_64-.+-(linux-gnu|freebsd.*)}}
# Inline assembly isn't supported by Memory Sanitizer
# UNSUPPORTED: msan
diff --git a/libunwind/test/signal_unwind.pass.cpp b/libunwind/test/signal_unwind.pass.cpp
index 4de271ecb886b..dd2bd9f2f1d5a 100644
--- a/libunwind/test/signal_unwind.pass.cpp
+++ b/libunwind/test/signal_unwind.pass.cpp
@@ -8,7 +8,7 @@
//===----------------------------------------------------------------------===//
// Ensure that the unwinder can cope with the signal handler.
-// REQUIRES: target={{(aarch64|loongarch64|riscv64|s390x|x86_64)-.+linux.*}}
+// REQUIRES: target={{(aarch64|loongarch64|riscv64|s390x|x86_64)-.+(linux|freebsd).*}}
// TODO: Figure out why this fails with Memory Sanitizer.
// XFAIL: msan
diff --git a/libunwind/test/unwind_leaffunction.pass.cpp b/libunwind/test/unwind_leaffunction.pass.cpp
index d336c159c131b..1c796b3433973 100644
--- a/libunwind/test/unwind_leaffunction.pass.cpp
+++ b/libunwind/test/unwind_leaffunction.pass.cpp
@@ -8,7 +8,7 @@
//===----------------------------------------------------------------------===//
// Ensure that leaf function can be unwund.
-// REQUIRES: target={{(aarch64|loongarch64|riscv64|s390x|x86_64)-.+linux.*}}
+// REQUIRES: target={{(aarch64|loongarch64|riscv64|s390x|x86_64)-.+(linux|freebsd).*}}
// TODO: Figure out why this fails with Memory Sanitizer.
// XFAIL: msan
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
arichardson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a lot of these should not have a requires Linux clause but instead list the unsupported targets.
It seems like a lot of the time they are actually checking for non-macos/non-windows. I wonder what happens when we drop the OS part of the triple from the test. Hopefully pre-commit CI will complain if this breaks something.
| // clang-format off | ||
|
|
||
| // REQUIRES: target={{x86_64-.+-linux-gnu}} | ||
| // REQUIRES: target={{x86_64-.+-(linux-gnu|freebsd.*)}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this was restricted to Linux in the first place. Maybe because of the echo issue you fixed here?
|
|
||
| // RUN: %{build} | ||
| // RUN: %{objcopy} --dump-section .eh_frame_hdr=%t_ehf_hdr.bin %t.exe | ||
| // RUN: echo -ne '\xFF' | dd of=%t_ehf_hdr.bin bs=1 seek=2 count=2 conv=notrunc status=none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this is still failing, echo should be a python built-in by default now. But fixing this to be more portable is good anyway (@boomanaiden154)
ac87d2d to
087dad9
Compare
|
I have removed all OS requirements from tests to see the CI fallout. I'll add |
… OSes While at it, replace "echo -ne" with "printf" for more compatibility.
087dad9 to
936acb2
Compare
arichardson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This new approach looks good to me assuming CI is happy.
|
I haven't even tried building |
arichardson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaxing the windows regexes sounds good and it looks like there is just one macos test failure so this should almost be ready to land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute is the reason the test fails on macos, since the syntax is different there. It was added in https://github.com/llvm/llvm-project/pull/85097/files. Apparently I even reviewed it but I don't recall doing so.
I think there are two options here:
- Add an XFAIL for macos
- Something like the following, which should work for GCC and Clang, but may not be portable to all architectures.
| #include <stdint.h> | |
| uintptr_t start_main; | |
| uintptr_t end_main; | |
| extern void foo(void); | |
| int main(int, char **) { | |
| start_main = (uintptr_t)&&func_start; | |
| end_main = (uintptr_t)&&func_end; | |
| func_start: | |
| foo(); | |
| return -2; | |
| func_end:; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing why the test is getting run at all on macos, despite me putting UNSUPPORTED: system-darwin?
There might be a cleaner way to enable these tests running on FreeBSD, I'm open to suggestions.